Fix issue with not being able to update connections on organizations #244

sergiught · 2022-07-16T07:37:00Z

Description

Fixes: #65

Checklist

Note: Checklist required to be completed before a PR is considered to be reviewable.

Auth0 Code of Conduct

I have read and agreed to the terms within the Auth0 Code of Conduct

Auth0 General Contribution Guidelines

I have read the Auth0 General Contribution Guidelines

Changes include test coverage?

Yes
Not needed

Does the description provide the correct amount of context?

Yes, the description provides enough context for the reviewer to understand what these changes accomplish

Have you updated the documentation?

Yes, I've updated the appropriate docs
Not needed

Is this code ready for production?

Yes, all code changes are intentional and no debugging calls are left over

sergiught · 2022-07-16T07:39:54Z

auth0/resource_auth0_organization.go

 						},
 					},
 				},
-				Set: hash.StringKey("connection_id"),


We can't use just the connection_id for the set hashes as that will ignore anything that we update on the assign_membership_on_login field. This is mostly the core of the fix, but in order to make it work properly several other things were needed. Please follow comments.

sergiught · 2022-07-16T07:40:22Z

auth0/resource_auth0_organization.go

@@ -64,7 +60,6 @@ func newOrganization() *schema.Resource {
 			"connections": {
 				Type:     schema.TypeSet,
 				Optional: true,
-				Computed: true,


This doesn't need to be a computed property, it's actually us within the config that decide the values of these and then it gets created on the API.

sergiught · 2022-07-16T07:40:47Z

auth0/resource_auth0_organization.go

@@ -74,83 +69,32 @@ func newOrganization() *schema.Resource {
 						"assign_membership_on_login": {
 							Type:     schema.TypeBool,
 							Optional: true,
+							Default:  false,


This default is very important to correctly figure out the differences on the hashed sets.

sergiught · 2022-07-16T07:46:32Z

auth0/resource_auth0_organization.go

 			},
 		},
 	}
 }

 func createOrganization(ctx context.Context, d *schema.ResourceData, m interface{}) diag.Diagnostics {
-	organization := buildOrganization(d)
+	organization := expandOrganization(d)


Renaming this so we're in line with the naming conventions "expand" and "flatten".

sergiught · 2022-07-16T07:48:14Z

auth0/resource_auth0_organization.go

 	api := m.(*management.Management)
 	if err := api.Organization.Create(organization); err != nil {
 		return diag.FromErr(err)
 	}

 	d.SetId(organization.GetID())

-	d.Partial(true)


Part of what caused the bug with the diffs I believe. The Partial func is a legacy func used when we had the SetPartial func as well (deprecated). This is not needed at all as on every command we run a refresh (read) even if we stop mid way here on the follow up plan or apply we'll read again and correct the state.

sergiught · 2022-07-16T07:51:09Z

auth0/resource_auth0_organization.go

+	toAdd, toRemove := Diff(d, "connections")
+
+	var err error
+	toRemove.Elem(func(data ResourceData) {


After we updated the schema we were getting at times weird errors about the connection already being there (409s).

This was because if we change the bool flag on a connection we get the following diff:

- connections { - assign_membership_on_login = false -> null - connection_id = "con_E68qtlsIB68K6Yz6" -> null } + connections { + assign_membership_on_login = true + connection_id = "con_E68qtlsIB68K6Yz6" }

So to correctly update the connection without adding complicated diffing logic, we delete the connection and then re-add it.

Update: We opted instead to create an internal map in the updateConnections function in order to not delete and then add even if the diff shows that in terraform. Unfortunately we couldn't suppress the diff correctly.

sergiught · 2022-07-16T07:52:24Z

auth0/resource_auth0_organization.go

+		return err
+	}
+
+	Set(d, "connections").Elem(func(data ResourceData) {


It was also important to remove the HasChange() from here, so that now we always loop through the final connections and send a PATCH with whatever the value is, as this operation is idempotent. This ultimately makes us correctly update the values.

sergiught · 2022-07-16T07:52:48Z

auth0/resource_auth0_organization_test.go

@@ -137,20 +99,114 @@ resource auth0_organization acme {
 }
 `

-const testAccOrganizationUpdateAgain = testAccOrganizationAux + `
+const testAccOrganizationUpdateAgain = testAccOrganizationGiven2Connections + `


Adding more thorough test scenarios and assertions.

willvedd · 2022-07-18T13:25:05Z

auth0/resource_auth0_organization_test.go

+					resource.TestCheckResourceAttr("auth0_organization.acme", "branding.0.colors.primary", "#e3e2f0"),
+					resource.TestCheckResourceAttr("auth0_organization.acme", "branding.0.colors.page_background", "#e3e2ff"),
+					resource.TestCheckResourceAttr("auth0_organization.acme", "metadata.%", "0"),
+					resource.TestCheckResourceAttr("auth0_organization.acme", "connections.#", "1"),


One thing I observed while working on the organization members is that for whatever reason, the bulk of this code had issues with completely clearing out members. It's possible that the connections has the same issue, so for that reason I think having the following test case would be useful:

resource.TestCheckResourceAttr("auth0_organization.acme", "connections.#", "0"),

I added another test case where we remove all connections 👍🏻 and tests pass

sergiught self-assigned this Jul 16, 2022

sergiught force-pushed the patch/fix-issue65-with-org-update branch 2 times, most recently from de8e692 to 0941fd7 Compare July 16, 2022 07:38

sergiught commented Jul 16, 2022

View reviewed changes

sergiught marked this pull request as ready for review July 16, 2022 07:53

sergiught requested a review from a team as a code owner July 16, 2022 07:53

sergiught force-pushed the patch/fix-issue65-with-org-update branch from 0941fd7 to 6f32c87 Compare July 16, 2022 09:38

Widcket approved these changes Jul 18, 2022

View reviewed changes

willvedd reviewed Jul 18, 2022

View reviewed changes

sergiught force-pushed the patch/fix-issue65-with-org-update branch from 6f32c87 to d030ba2 Compare July 18, 2022 14:44

Widcket approved these changes Jul 18, 2022

View reviewed changes

Fix issue with not being able to update connections on organizations

bfb8cf3

sergiught force-pushed the patch/fix-issue65-with-org-update branch from d030ba2 to bfb8cf3 Compare July 18, 2022 15:28

sergiught merged commit bd683cd into main Jul 18, 2022

sergiught deleted the patch/fix-issue65-with-org-update branch July 18, 2022 15:32

sergiught mentioned this pull request Jul 18, 2022

Organization connection does not get updated #65

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix issue with not being able to update connections on organizations #244

Fix issue with not being able to update connections on organizations #244

sergiught commented Jul 16, 2022 •

edited

Loading

sergiught Jul 16, 2022

sergiught Jul 16, 2022

sergiught Jul 16, 2022

sergiught Jul 16, 2022

sergiught Jul 16, 2022

sergiught Jul 16, 2022

sergiught Jul 18, 2022

sergiught Jul 16, 2022

sergiught Jul 16, 2022

willvedd Jul 18, 2022

sergiught Jul 18, 2022

Fix issue with not being able to update connections on organizations #244

Fix issue with not being able to update connections on organizations #244

Conversation

sergiught commented Jul 16, 2022 • edited Loading

Description

Checklist

Auth0 Code of Conduct

Auth0 General Contribution Guidelines

Changes include test coverage?

Does the description provide the correct amount of context?

Have you updated the documentation?

Is this code ready for production?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sergiught commented Jul 16, 2022 •

edited

Loading